Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: beacon depends on validator and bridge #28

Merged
merged 7 commits into from
Dec 17, 2024

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Dec 11, 2024

Closes #24

The whole stack now reliably starts

@rootulp rootulp self-assigned this Dec 11, 2024
@rootulp rootulp changed the title fix: beacon depends on validator fix: beacon depends on validator and bridge Dec 11, 2024
Comment on lines 13 to 19
depends_on:
- celestia-network-bridge
- reth
celestia-network-validator:
condition: service_healthy
celestia-network-bridge:
condition: service_started
reth:
condition: service_started
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this makes any logical difference as the celestia-network-bridge doesn't start until the validator is "healthy" anyway.

If you wanted to add a health check to the bridge node, you could try using some lightweight method like /network_head.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I tried to add a health check using node.Info or p2p.Info but both required an admin token. Looks like /network_head only needs a read auth token so seems a tad easier.

Ref: https://node-rpc-docs.celestia.org/?version=v0.20.4

Created celestiaorg/celestia-node#4007 so celestia-node exposes an un authenticated status endpoint. In the meantime DevOps shared a workaround that they use:

exec [sh -c AUTH_TOKEN=$(celestia bridge auth read --node.store=/home/celestia 2>/dev/null); curl --max-time 0.5 -s -X POST -H "Content-type: application/json" -H "Accept: application/json" -H "Authorization: Bearer $AUTH_TOKEN" -d '{"id":1,"jsonrpc":"2.0","method":"header.SyncWait","params":[]}' "http://localhost:26658"; if [ "$?" -eq 0 ]; then exit 0; else echo "Catching up"; exit 1; fi ]

so I'll try something like that

@rootulp rootulp marked this pull request as draft December 12, 2024 15:41
@rootulp
Copy link
Collaborator Author

rootulp commented Dec 16, 2024

A couple of findings:

  • We need to use two $ to reference an environment variable in the healthcheck command. Example: $$FOO
  • I can't hit the RPC server of the celestia-node instance

@rootulp
Copy link
Collaborator Author

rootulp commented Dec 16, 2024

Update the celestia bridge node is starting prematurely. If I retry starting it after the validator has been up for some time, it works. However, on initial launch it doesn't work and fails to start the RPC server.

The healthcheck on the validator doesn't seem to be granular enough.

Comment on lines 58 to 61
test: ["CMD-SHELL", "/bin/sh -c 'BLOCK_HEIGHT=$(curl -sf http://localhost:26657/status | jq -r .result.sync_info.latest_block_height); if [ \"$$BLOCK_HEIGHT\" -gt 1 ]; then exit 0; else echo \"Block height too low: $$BLOCK_HEIGHT\"; exit 1; fi'"]
interval: 1s
timeout: 5s
retries: 5
retries: 20
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note for reviewers] this makes Docker report this service healthy only if the block height is > 1

@rootulp rootulp marked this pull request as ready for review December 16, 2024 23:19
@rootulp rootulp requested a review from cmwaters December 16, 2024 23:19
Copy link
Member

@ninabarbakadze ninabarbakadze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please push your branch to the repo because i can't fetch it unless i set your fork as upstream 🙏🏻

@rootulp
Copy link
Collaborator Author

rootulp commented Dec 17, 2024

You can fetch with this thing without setting my fork as upstream.

Update: also pushed this branch to upstream

Screenshot 2024-12-17 at 9 07 35 AM

Copy link
Collaborator

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran this a couple times and worked for me every time

@ninabarbakadze
Copy link
Member

You can fetch with this thing without setting my fork as upstream.

Update: also pushed this branch to upstream

Screenshot 2024-12-17 at 9 07 35 AM

thanks! didn't know you could do this

@rootulp rootulp merged commit ec2ac9d into celestiaorg:main Dec 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make docker compose up --detach reliable
3 participants